Skip to content

Hide kuzzle object from console.log #520

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Jun 29, 2020
Merged

Hide kuzzle object from console.log #520

merged 11 commits into from
Jun 29, 2020

Conversation

Aschen
Copy link
Contributor

@Aschen Aschen commented Jun 9, 2020 •
edited
Loading

What does this PR do?

Use Reflect to hide the kuzzle object from log

Other changes

  • Get ride of every delete keyword and use Map for RAM caches
  • Do not throw custom error when unsubscribing from unknown room (let Kuzzle trigger it's own documented error)
  • Allows to pass _id to realtime.publish

@Aschen Aschen changed the base branch from master to 7-dev June 9, 2020 04:33
@Aschen Aschen self-assigned this Jun 9, 2020
Aschen
Aschen commented Jun 9, 2020
View reviewed changes
Aschen
Aschen commented Jun 9, 2020
View reviewed changes
@Aschen Aschen mentioned this pull request Jun 9, 2020
Merged
@codecov
Copy link

codecov bot commented Jun 9, 2020 •
edited
Loading

Codecov Report

Merging #520 into 7-dev will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            7-dev     #520      +/-   ##
==========================================
- Coverage   95.59%   95.57%   -0.03%     
==========================================
  Files          32       32              
  Lines        1318     1311       -7     
==========================================
- Hits         1260     1253       -7     
  Misses         58       58
Impacted Files Coverage Δ
src/controllers/MemoryStorage.js 96.05% <ø> (-0.20%) ⬇️
src/controllers/Security.js 97.03% <ø> (-0.03%) ⬇️
src/controllers/Base.js 100.00% <100.00%> (ø)
src/controllers/Realtime.js 97.87% <100.00%> (+0.04%) ⬆️
src/core/KuzzleEventEmitter.js 96.42% <100.00%> (+0.06%) ⬆️
src/core/Room.js 100.00% <100.00%> (ø)
src/core/searchResult/SearchResultBase.js 90.19% <100.00%> (ø)
src/core/security/Profile.js 100.00% <100.00%> (ø)
src/core/security/Role.js 80.00% <100.00%> (ø)
src/core/security/User.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6d154d...d4a1d26. Read the comment docs.

Leodau
Leodau approved these changes Jun 9, 2020
View reviewed changes
scottinet
scottinet suggested changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@scottinet scottinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple issues:

  • you should mention what versions of kuzzle accept this option
  • this looks like a circular dependency in disguise; why do we need to release the support for this option before we release the version of kuzzle accepting it?

@Aschen
Copy link
Contributor Author

Aschen commented Jun 9, 2020

A couple issues:

* you should mention what versions of kuzzle accept this option

* this looks like a circular dependency in disguise; why do we need to release the support for this option before we release the version of kuzzle accepting it?

I'm still not sure about what Kuzzle version it's gonna be. It's an enhancement since the SDK is already exposed but I still feel like being able to use it with the HotelClerc features is a big improvement and it may deserve the new-feature tag IMHO, what do you think ?

About the circular dependency, I have no choice unfortunately because I cannot use the query method since I need to use the realtime.subscribe method with the Room mechanism.. (It would have been possible with this feature: #489 😁 )

But in this case it's not a big deal since this option is not recognized by Kuzzle for now.

@scottinet
Copy link
Contributor

What do you think about hiding the documentation (you can left it there, but comment it), so that once Kuzzle is released with that feature, you can uncomment the SDK documentation, adding the right Kuzzle version (and a link to the appropriate guide/API doc)?

I fear that documenting a feature that cannot be used (at the moment) is not a good idea.

@Aschen
Copy link
Contributor Author

Aschen commented Jun 9, 2020

@scottinet I'm OK with that.
And also maybe we should not document this feature at all in the SDK but only in the plugin/application documentation since the options is useless outside a plugin, what do you think?

@Aschen Aschen force-pushed the allow-cluster-arg-for-subscribe branch from 697f88a to d1dbcfe Compare June 10, 2020 03:11
@Aschen Aschen changed the title Allows cluster arg for realtime.subscribe Hide kuzzle object from console.log Jun 10, 2020
@Aschen Aschen requested review from Leodau and scottinet June 10, 2020 10:12
@Aschen
Copy link
Contributor Author

Aschen commented Jun 10, 2020

@Leodau @scottinet
I removed all the code related to the cluster option since this option is now added by the EmbeddedSDK only.
So this PR is just about few minor stuff now

scottinet
scottinet suggested changes Jun 11, 2020
View reviewed changes
src/core/security/Role.js Outdated
this._kuzzle = kuzzle;
Reflect.defineProperty(this, '_kuzzle', {
value: kuzzle,
writable: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this property writable?

@scottinet scottinet merged commit 6efa1dd into 7-dev Jun 29, 2020
@scottinet scottinet deleted the allow-cluster-arg-for-subscribe branch June 29, 2020 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Leodau Leodau Leodau approved these changes

@scottinet scottinet scottinet approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Aschen Aschen

Labels

7-dev changelog:exclude

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Aschen @scottinet @Leodau